Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v18.x] Temporarily revert libuv #50036

Closed

Conversation

richardlau
Copy link
Member

@richardlau richardlau commented Oct 4, 2023

Temporarily revert recent libuv updates (1.46.0 and 1.45.0) in Node.js 18 to fix regressions:

Also #49911 which is a separate bug that was exposed by the performance profile change that IO_URING introduced (for this particular one I've already landed the Node.js fix on the staging branch but it demonstrates the risk that drastic performance changes can have on LTS release lines).

While the IO_URING issues can be worked around by setting UV_USE_IO_URING=0 in the environment, the Windows FS regression cannot. Upstream libuv contains fixes for the Windows regression and IO_URING fixes that are not yet in a libuv release:

I think it's too early to have IO_URING enabled by default in LTS Node.js. Were it just the IO_URING issues I might propose patching libuv to disable it by default in Node.js 18 instead of reverting the updates.

Even if a new libuv version was to be made available now we normally would not land that in LTS until it has been released in a current release for two weeks.

cc @nodejs/lts @nodejs/releasers

@richardlau richardlau added libuv Issues and PRs related to the libuv dependency or the uv binding. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Oct 4, 2023
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 4, 2023
@richardlau richardlau added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Oct 4, 2023
@richardlau
Copy link
Member Author

For transparency, reverting libuv 1.45.0 and 1.46.0 would undo the changes noted in

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubber stamp LGTM

@targos
Copy link
Member

targos commented Oct 4, 2023

What do you think is the risk to break users who might depend on the new libuv versions being present?

@mhdawson
Copy link
Member

mhdawson commented Oct 4, 2023

@targos are you thinking about native addons or something else in terms of dependencies on newer versions of libuv?

@targos
Copy link
Member

targos commented Oct 4, 2023

Nothing in particular. Could be native addons. Could be bug fixes that affect specific workloads and are now depended upon.
I think disabling IO_URING should be safe, but we need to be careful with full reverts. The vast majority of production deployments are not on Windows.

@richardlau
Copy link
Member Author

What do you think is the risk to break users who might depend on the new libuv versions being present?

Low? Of the notable changes in:

Node.js 18 has had regressions for the last three releases

Reverting the libuv change would be a fairly quick way of fixing the regressions in Node.js 18.18.0. I can volunteer to do a quick Node.js 18.18.1 release containing this PR -- I would not have time to volunteer to do a full Node.js 18 release this month.

If people would rather we did not temporarily revert libuv in Node.js 18 please request changes on this PR. I'm happy to go with whatever consensus @nodejs/lts comes to.

Copy link
Member

@santigimeno santigimeno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me. Will try to cut a new libuv release with those fixes soonish.

@nodejs-github-bot
Copy link
Collaborator

@arsnyder16
Copy link

arsnyder16 commented Oct 4, 2023

Are these issues also present in node 20? Is there a plan to revert libuv in 20 as well?

I am just wondering what the options are for cgroup v2 and LTS

@richardlau
Copy link
Member Author

Are these issues also present in node 20? Is there a plan to revert libuv in 20 as well?

I am just wondering what the options are for cgroup v2 and LTS

Currently no plans to revert in Node.js 20 -- the libuv updates have been in eight Node.js 20 releases (starting with 20.3.0 in June).

richardlau added a commit that referenced this pull request Oct 6, 2023
This reverts commit ea23870.

PR-URL: #50036
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
richardlau added a commit that referenced this pull request Oct 6, 2023
This reverts commit 7dc731d.

PR-URL: #50036
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
richardlau added a commit that referenced this pull request Oct 6, 2023
This reverts commit 88855e0.

PR-URL: #50036
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
richardlau added a commit that referenced this pull request Oct 6, 2023
This reverts commit fb2b80f.

PR-URL: #50036
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
@richardlau
Copy link
Member Author

Landed in 7a3e1ff...14ece2c.

@richardlau richardlau closed this Oct 6, 2023
@richardlau richardlau deleted the v18.x-revert-libuv branch October 6, 2023 16:03
This was referenced Oct 6, 2023
codebytere added a commit to electron/electron that referenced this pull request Oct 11, 2023
codebytere added a commit to electron/electron that referenced this pull request Oct 11, 2023
codebytere added a commit to electron/electron that referenced this pull request Oct 12, 2023
* chore: bump node in DEPS to v18.18.1

* Revert "deps: upgrade to libuv 1.46.0"

nodejs/node#50036

* chore: fixup patch indices

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
baparham pushed a commit to baparham/electron that referenced this pull request Nov 7, 2023
* chore: bump node in DEPS to v18.18.1

* Revert "deps: upgrade to libuv 1.46.0"

nodejs/node#50036

* chore: fixup patch indices

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Nov 9, 2023
* chore: bump node to v18.18.1 (main) (#40174)

* chore: bump node in DEPS to v18.18.1

* Revert "deps: upgrade to libuv 1.46.0"

nodejs/node#50036

* chore: fixup patch indices

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

* chore: bump node to v18.18.2 (main) (#40205)

* chore: bump node in DEPS to v18.18.2

* chore: update patches

* deps: update nghttp2 to 1.55.0

nodejs/node#48746

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
* chore: bump node in DEPS to v18.18.1

* Revert "deps: upgrade to libuv 1.46.0"

nodejs/node#50036

* chore: fixup patch indices

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libuv Issues and PRs related to the libuv dependency or the uv binding. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants